[https://nvbugs/5540752][fix] Support quantized Phi4 MM models #8190
Conversation
|
/bot run |
|
PR_Github #20777 [ run ] triggered by Bot |
|
PR_Github #20777 [ run ] completed with state |
|
Hey @pamelap-nvidia great work, could we maybe add a e2e test on b200s? on sm120 post merge would be nice too. AFAIR, these changes also unblocked phi4-reason-plus fp4 as well when we did for spark-dev branch. Might be good to test that too. |
pamelap-nvidia
force-pushed
the
phi4_fp4
branch
from
511ff94 to
60895b0
Compare
|
/bot run --extra-stage "RTXPro6000-PyTorch-Post-Merge-1,RTXPro6000-4_GPUs-PyTorch-Post-Merge-1,RTXPro6000-4_GPUs-PyTorch-Post-Merge-2" |
|
PR_Github #21325 [ run ] triggered by Bot |
📝 WalkthroughWalkthroughUpdates weight-loading logic for Phi-3 (QKV and MLP gate/up with scale handling), modifies Phi-4MM dynamic import and base-layer key remapping, adjusts multimodal input modality handling, and expands tests and references to include FP4/FP8 variants across accuracy, perf, e2e, and test lists. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Loader as WeightLoader (Phi3)
participant State as StateDict
participant Layer as Attention/MLP Layer
Note over Loader,Layer: QKV loading with per-part scales
Loader->>State: get qkv_weight, weight_scale, input_scale, weight_scale_2
alt explicit indices
Loader->>Loader: split q[:qk], k[qk:kv], v[kv:]
end
alt weight_scale dims match qkv
Loader->>Layer: set q/k/v weights with respective weight_scale slices
else shared scale
Loader->>Layer: set q/k/v weights with shared weight_scale
end
opt input_scale / weight_scale_2 present
Loader->>Layer: attach input_scale, weight_scale_2 to q/k/v
end
Note over Loader,Layer: gate_up split in MLP
Loader->>State: get gate_up_weight (+scales)
Loader->>Loader: split into gate, up
alt weight_scale dims match
Loader->>Layer: set gate/up with respective scale slices
else shared scale
Loader->>Layer: set gate/up with shared scale
end
opt input_scale / weight_scale_2 present
Loader->>Layer: attach to gate/up
end
sequenceDiagram
autonumber
participant Init as Phi4MM Init
participant FS as Filesystem
participant Import as Importlib
participant Model as Phi4MM Model
Note over Init,Import: Dynamic module resolution
Init->>FS: derive package_name from local_path
Init->>Import: import {package_name}.modeling_phi4mm as hf_modeling_phi4mm
Import-->>Init: module
Note over Init,Model: Base-layer key remap on load
Init->>Model: load_weights(state_dict)
Model->>Model: for k in keys: replace "base_layer.{layer}" with "{layer}" for {weight,input_scale,weight_scale,weight_scale_2}
Model-->>Init: weights loaded
sequenceDiagram
autonumber
participant Util as convert_to_conversation_message
participant Tracker as mm_data_tracker
Util->>Util: mdata is dict? extract modality->mdata_modality
Util->>Util: if mdata_modality == "multiple_image" set to "image"
Util->>Tracker: add_data(mdata_modality, data)
Note over Util: Current patch references undefined "modality" in condition path (potential NameError)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tensorrt_llm/_torch/models/modeling_phi4mm.py (1)
1-1: Add SPDX header (compliance).Prepend the NVIDIA Apache-2.0 copyright header with current year to this Python file.
Apply at file top:
+# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +#As per coding guidelines
tensorrt_llm/_torch/models/modeling_phi3.py (1)
1-1: Add SPDX header (compliance).Prepend the NVIDIA Apache-2.0 header to this source file.
+# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +#As per coding guidelines
tensorrt_llm/inputs/utils.py (1)
583-590: Fix modality remap for multiple-image (also for embeddings); use per-item modality.Current code checks the outer loader
modalityand only remaps for data, not embeddings. For multiple_image embeddings this will produce unknown modality in placeholder lookup. Normalize by item and apply to both paths.Apply:
- mdata_modality = mdata["modality"] - if modality == "multiple_image": - mdata_modality = "image" - mm_data_tracker.add_data(mdata_modality, mdata["data"]) + mdata_modality = mdata["modality"] + if mdata_modality == "multiple_image": + mdata_modality = "image" + mm_data_tracker.add_data(mdata_modality, mdata["data"]) else: # Add embeddings to the tracker for placeholder handling - mm_data_tracker.add_data(mdata["modality"], - mdata["mm_embedding_info"]) + em_modality = mdata["modality"] + if em_modality == "multiple_image": + em_modality = "image" + mm_data_tracker.add_data(em_modality, + mdata["mm_embedding_info"])
🧹 Nitpick comments (3)
tensorrt_llm/_torch/models/modeling_phi4mm.py (1)
1005-1007: Derive mm_token_ids device from model params to avoid mismatches.Using self.device may be undefined or on a different device than the model/inputs. Bind to the LLM’s actual parameter device.
Apply:
- self.mm_token_ids = torch.tensor( - [_IMAGE_SPECIAL_TOKEN_ID, _AUDIO_SPECIAL_TOKEN_ID], - device=self.device) + device = next(self.llm.parameters()).device + self.mm_token_ids = torch.tensor( + [_IMAGE_SPECIAL_TOKEN_ID, _AUDIO_SPECIAL_TOKEN_ID], + device=device)tensorrt_llm/inputs/utils.py (1)
603-611: Minor: avoid shadowing built-in nameinput.Rename local variable
inputtoinputs_dictorpayloadto improve readability.- input = {"prompt": prompt} - if mm_placeholder_counts: - if mm_embeddings is not None: - input[ - "multi_modal_embeddings"] = mm_data_tracker.retrieve_all_sync( - ) - else: - input["multi_modal_data"] = mm_data_tracker.retrieve_all_sync() - inputs.append(input) + inputs_dict = {"prompt": prompt} + if mm_placeholder_counts: + if mm_embeddings is not None: + inputs_dict["multi_modal_embeddings"] = mm_data_tracker.retrieve_all_sync() + else: + inputs_dict["multi_modal_data"] = mm_data_tracker.retrieve_all_sync() + inputs.append(inputs_dict)tests/integration/defs/accuracy/test_llm_api_pytorch.py (1)
3286-3303: Add quant-algo assertions for early validation.Assert the expected quantization for FP4/FP8 variants to catch misconfigured paths faster.
@skip_pre_blackwell def test_fp4(self): model_path = f"{self.MODEL_PATH}-FP4" with LLM(model_path, max_seq_len=4096) as llm: + assert llm.args.quant_config.quant_algo == QuantAlgo.NVFP4 task = MMLU(self.MODEL_NAME) task.evaluate(llm) task = GSM8K(self.MODEL_NAME) task.evaluate(llm) @skip_pre_hopper def test_fp8(self): model_path = f"{self.MODEL_PATH}-FP8" with LLM(model_path, max_seq_len=4096) as llm: + assert llm.args.quant_config.quant_algo == QuantAlgo.FP8 task = MMLU(self.MODEL_NAME) task.evaluate(llm) task = GSM8K(self.MODEL_NAME) task.evaluate(llm)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
tensorrt_llm/_torch/models/modeling_phi3.py(2 hunks)tensorrt_llm/_torch/models/modeling_phi4mm.py(3 hunks)tensorrt_llm/inputs/utils.py(1 hunks)tests/integration/defs/accuracy/references/gsm8k.yaml(1 hunks)tests/integration/defs/accuracy/references/mmlu.yaml(1 hunks)tests/integration/defs/accuracy/test_llm_api_pytorch.py(1 hunks)tests/integration/defs/perf/test_perf.py(2 hunks)tests/integration/defs/test_e2e.py(18 hunks)tests/integration/test_lists/qa/llm_function_core.txt(2 hunks)tests/integration/test_lists/qa/llm_function_l20.txt(2 hunks)tests/integration/test_lists/qa/llm_function_rtx6k.txt(2 hunks)tests/integration/test_lists/test-db/l0_l40s.yml(1 hunks)tests/integration/test_lists/test-db/l0_rtx_pro_6000.yml(2 hunks)tests/integration/test_lists/waives.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{h,hpp,hh,hxx,cpp,cxx,cc,cu,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Use only spaces, no tabs; indent with 4 spaces.
Files:
tests/integration/defs/accuracy/test_llm_api_pytorch.pytensorrt_llm/_torch/models/modeling_phi4mm.pytensorrt_llm/_torch/models/modeling_phi3.pytests/integration/defs/perf/test_perf.pytensorrt_llm/inputs/utils.pytests/integration/defs/test_e2e.py
**/*.py
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+.
Indent Python code with 4 spaces; do not use tabs.
Maintain module namespace when importing; prefer 'from package.subpackage import foo' then 'foo.SomeClass()' instead of importing the class directly.
Python filenames should be snake_case (e.g., some_file.py).
Python classes use PascalCase names.
Functions and methods use snake_case names.
Local variables use snake_case; prefix 'k' for variables that start with a number (e.g., k_99th_percentile).
Global variables use upper SNAKE_CASE prefixed with 'G' (e.g., G_MY_GLOBAL).
Constants use upper SNAKE_CASE (e.g., MY_CONSTANT).
Avoid shadowing variables from an outer scope.
Initialize all externally visible members of a class in the constructor.
Prefer docstrings for interfaces that may be used outside a file; comments for in-function or file-local interfaces.
Use Google-style docstrings for classes and functions (Sphinx-parsable).
Document attributes and variables inline so they render under the class/function docstring.
Avoid reflection when a simpler, explicit approach suffices (e.g., avoid dict(**locals()) patterns).
In try/except, catch the most specific exceptions possible.
For duck-typing try/except, keep the try body minimal and use else for the main logic.
Files:
tests/integration/defs/accuracy/test_llm_api_pytorch.pytensorrt_llm/_torch/models/modeling_phi4mm.pytensorrt_llm/_torch/models/modeling_phi3.pytests/integration/defs/perf/test_perf.pytensorrt_llm/inputs/utils.pytests/integration/defs/test_e2e.py
**/*.{cpp,cxx,cc,h,hpp,hh,hxx,cu,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Prepend the NVIDIA Apache-2.0 copyright header with current year to the top of all source files (e.g., .cpp, .h, .cu, .py).
Files:
tests/integration/defs/accuracy/test_llm_api_pytorch.pytensorrt_llm/_torch/models/modeling_phi4mm.pytensorrt_llm/_torch/models/modeling_phi3.pytests/integration/defs/perf/test_perf.pytensorrt_llm/inputs/utils.pytests/integration/defs/test_e2e.py
🧠 Learnings (1)
📚 Learning: 2025-09-17T02:48:52.732Z
Learnt from: tongyuantongyu
PR: NVIDIA/TensorRT-LLM#7781
File: tests/integration/test_lists/waives.txt:313-313
Timestamp: 2025-09-17T02:48:52.732Z
Learning: In TensorRT-LLM, `tests/integration/test_lists/waives.txt` is specifically for waiving/skipping tests, while other test list files like those in `test-db/` and `qa/` directories are for different test execution contexts (pre-merge, post-merge, QA tests). The same test appearing in both waives.txt and execution list files is intentional - the test is part of test suites but will be skipped due to the waiver.
Applied to files:
tests/integration/test_lists/waives.txt
🧬 Code graph analysis (2)
tests/integration/defs/accuracy/test_llm_api_pytorch.py (2)
tensorrt_llm/llmapi/llm.py (1)
LLM(1084-1100)tests/integration/defs/accuracy/accuracy_core.py (4)
MMLU(317-331)evaluate(184-247)evaluate(765-775)GSM8K(334-349)
tests/integration/defs/test_e2e.py (2)
tensorrt_llm/llmapi/llm_args.py (1)
model_name(1363-1364)tests/integration/defs/disaggregated/test_disaggregated_single_gpu.py (1)
model_path(75-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (16)
tests/integration/test_lists/qa/llm_function_rtx6k.txt (2)
22-25: Good coverage expansion for Phi4MM accuracy.Entries align with added tests. Please confirm node ids match pytest param ids after collection.
48-56: E2E multimodal params look consistent with new signature.Looks right. Verify collected ids match these strings (collect-only) to avoid list drift.
tests/integration/test_lists/test-db/l0_l40s.yml (1)
25-27: Renamed Phi4MM tests look correct.Matches updated model_name/model_path/modality tuple usage.
tests/integration/test_lists/waives.txt (1)
352-353: Waivers updated to new Phi4MM ids.Consistent with the intent of waives.txt to skip tests while keeping them listed elsewhere. Based on learnings
tests/integration/defs/accuracy/references/mmlu.yaml (1)
297-300: Added reference accuracies for Phi‑4 MM FP8/NVFP4.Looks fine. Please record dataset/seed and eval config used to generate these for reproducibility.
tests/integration/test_lists/test-db/l0_rtx_pro_6000.yml (1)
38-40: Adds Phi4MM FP4/FP8 tests and 2‑GPU variants.Looks good. Please sanity‑check pytest collection so ids match these entries.
Also applies to: 44-45, 110-111
tests/integration/defs/test_e2e.py (2)
2777-2782: LGTM on FP4/FP8 support and Phi4MM handling.
- startswith('phi4‑multimodal‑instruct') gating looks good.
- Param expansions for model_name/model_path consistent across tests.
Also applies to: 2960-2965, 2981-2992, 3179-3185, 3194-3195
2659-2666: LGTM on added KV‑reuse/chunked‑prefill coverage and expectations.Additions are coherent with new variants and skip rules.
Also applies to: 2685-2688, 2741-2756, 2810-2816, 2835-2838, 2916-2944
tensorrt_llm/_torch/models/modeling_phi4mm.py (2)
91-116: Dynamic module name: check import stability with hyphenated package names.Deriving package_name from the folder is fine. Please verify that relative imports in HF’s modeling_phi4mm.py still resolve when package_name contains hyphens (e.g., Phi-4-multimodal-instruct[-FP4]) and that repeated loads don’t collide in sys.modules.
991-1001: Base-layer key remap looks good.Mapping base_layer.{weight,input_scale,weight_scale,weight_scale_2} → direct keys simplifies load_weights and aligns with Phi3 handling.
Please double-check for accidental key collisions when both base_layer.* and non-base-layer keys coexist in the same module path.
tensorrt_llm/_torch/models/modeling_phi3.py (1)
220-229: Q/K/V split boundaries look correct.q: [:hidden_size], k: [hidden_size:hidden_size+num_kv_heads*head_dim], v: [rest] matches expected shapes.
tests/integration/defs/perf/test_perf.py (2)
130-137: Perf model-path additions look consistent.New phi-4 multimodal FP4/FP8 entries align with existing naming and local repo layout.
188-195: LoRA-path additions look correct.LoRA directories for FP4/FP8 variants mirror the base model entries.
tests/integration/test_lists/qa/llm_function_core.txt (1)
602-604: LGTM — core QA list updated for Phi‑4 MM FP4/FP8.Entries align with new tests and naming.
Also applies to: 667-675, 677-686, 689-691
tests/integration/defs/accuracy/references/gsm8k.yaml (1)
190-193: MMLU references for Phi-4 MM FP8/NVFP4 confirmed
Entries for microsoft/Phi-4-multimodal-instruct with both quant_algo: FP8 and quant_algo: NVFP4 exist in tests/integration/defs/accuracy/references/mmlu.yaml.tests/integration/test_lists/qa/llm_function_l20.txt (1)
44-46: LGTM — coverage extended to Phi-4 MM FP4/FP8 and end-to-end tests. TestsTestPhi4MM.test_fp4/test_fp8andtest_ptp_quickstart_multimodal_phi4mmentries confirmed present.
|
PR_Github #21325 [ run ] completed with state |
pamelap-nvidia
force-pushed
the
phi4_fp4
branch
from
24ccf26 to
3ee0b99
Compare
|
/bot run --extra-stage "RTXPro6000-PyTorch-Post-Merge-1,RTXPro6000-4_GPUs-PyTorch-Post-Merge-1,RTXPro6000-4_GPUs-PyTorch-Post-Merge-2" |
|
PR_Github #21563 [ run ] triggered by Bot |
|
PR_Github #21563 [ run ] completed with state |
pamelap-nvidia
force-pushed
the
phi4_fp4
branch
from
3ee0b99 to
b7a041f
Compare
|
/bot run --extra-stage "RTXPro6000-PyTorch-Post-Merge-1,RTXPro6000-4_GPUs-PyTorch-Post-Merge-1,RTXPro6000-4_GPUs-PyTorch-Post-Merge-2" |
|
PR_Github #21830 [ run ] triggered by Bot. Commit: |
|
PR_Github #21830 [ run ] completed with state |
Signed-off-by: Pamela <[email protected]> update tokenizer and processor Signed-off-by: Pamela <[email protected]> revert WAR for shapes Signed-off-by: Pamela <[email protected]> fix fp8 scale Signed-off-by: Pamela <[email protected]> fix image_audio Signed-off-by: Pamela <[email protected]>
Signed-off-by: Pamela <[email protected]>
Signed-off-by: Pamela <[email protected]>
Signed-off-by: Pamela <[email protected]>
pamelap-nvidia
force-pushed
the
phi4_fp4
branch
from
b7a041f to
872c7b2
Compare
|
/bot reuse-pipeline |
|
PR_Github #21896 [ reuse-pipeline ] triggered by Bot. Commit: |
|
PR_Github #21896 [ reuse-pipeline ] completed with state |
…A#8190) Signed-off-by: Pamela <[email protected]>
5 participants
Support quantized phi4 MM models
Added tests for FP4 and FP8 ckpts.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.